Event: Warn and fill event aliases #243
Conversation
|
@dmethvin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @mgol and @LaurentBarbareau to be potential reviewers. |
| @@ -199,3 +199,15 @@ See jQuery-ui [commit](https://github.com/jquery/jquery-ui/commit/c0093b599fcd58 | |||
| **Cause:** The calling code has attempted to attach a `load` event to `window` after the page has already loaded. That means the handler will never run and so is probably not what the caller intended. This can occur when the event attachment is made too late, for example, in a jQuery ready handler. It can also occur when a file is loaded dynamically with jQuery after the page has loaded, for example using the `$.getScript()` method. | |||
|
|
|||
| **Solution:** If a function `fn` does not actually depend on all page assets being fully loaded, switch to a ready handler `$( fn )` which runs earlier and will aways run `fn` even if the script that contains the code loads long after the page has fully loaded. If `fn` actually does depend on the script being fully loaded, check `document.readyState`. If the value is `"complete"` run the function immediately, otherwise use `$(window).on( "load", fn )`. | |||
|
|
|||
| ### JQMIGRATE: jQuery.fn.click() event shorthand is deprecated | |||
There was a problem hiding this comment.
Why not generalize this header?
There was a problem hiding this comment.
I tried to figure out the best way to do this. My concern with generalizing the header was that it wouldn't be an exact match. The most common shorthand (I think) is click so i figured this approach would yield the best results with the others in the text. Another way is to duplicate the exact messages but that would get out of hand with so many on the list now. Or I can change it to something like METHOD. What do you think?
| // Handle event binding | ||
| jQuery.fn[ name ] = function( data, fn ) { | ||
| migrateWarn( "jQuery.fn." + name + "() event shorthand is deprecated" ); | ||
| return arguments.length > 0 ? |
There was a problem hiding this comment.
Can we remember the old functions and call them instead duplicating logic in core?
There was a problem hiding this comment.
What do you mean? This code should work even if event shorthands are removed from Core.
There was a problem hiding this comment.
As far as i know we don't plan to remove it. And if/when we remove it, warning should be changed regardless
There was a problem hiding this comment.
It will be missing if someone compiles the deprecated module out. @dmethvin does Migrate support such a scenario?
There was a problem hiding this comment.
It seems reimplementing is more preferable, judging by the rest of the source anyway
There was a problem hiding this comment.
I think our approach for 3.0 has been to implement the deprecated functionality even if it's still in core. It does handle the scenario of custom builds better i guess.
| "scroll click submit keydown".split( " " ).forEach( function( name ) { | ||
| expectWarning( "." + name + "()", 1, function() { | ||
| $div[ name ]( function( event ) { | ||
| assert.equal( event.type, name, name) |
There was a problem hiding this comment.
...and a missing semi-colon at the end. :)
There was a problem hiding this comment.
Oh great now I have to go figure out why the eslint settings missed that. :)
| @@ -94,6 +94,37 @@ QUnit.test( ".delegate() and .undelegate()", function( assert ) { | |||
| } ); | |||
| } ); | |||
|
|
|||
| QUnit.test( "Event aliases", function( assert) { | |||
| assert.expect( 16 ); | |||
There was a problem hiding this comment.
Where does the 16 come from? I counted 14:
- in the first block: 4 warnings & 4
assert.equals - in the second block: 1 warning & 2
assert.oks - in the third block: 1 warning & 2
assert.equals
What am I missing?
There was a problem hiding this comment.
Oh man, you're right. It was picking up 2 extra from previously attached event handlers. Nice catch!
Fixes #230